-
Notifications
You must be signed in to change notification settings - Fork 156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(tfhe): add zk-pok code base #1050
Conversation
IceTDrinker
commented
Apr 4, 2024
- integration of work done by Sarah in the repo
12a3adf
to
ccbd64d
Compare
480907d
to
24a579e
Compare
cuda C API tests likely need the ZK pok feature @tmontaigu |
24a579e
to
e337007
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments that were waiting apparently, will continue this afternoon
tfhe/Cargo.toml
Outdated
@@ -69,6 +69,8 @@ paste = "1.0.7" | |||
fs2 = { version = "0.4.3", optional = true } | |||
# While we wait for repeat_n in rust standard library | |||
itertools = "0.11.0" | |||
rand_core = { version = "0.6.4", features = ["std"] } | |||
tfhe-zk-pok = { version = "0.1.0", path = "../tfhe-zk-pok", optional = true} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatting, missing space before } , we'll add toml formatter at some point I guess but I don't think I found one that's easy to setup with e.g. rust
tfhe/Cargo.toml
Outdated
@@ -270,6 +273,10 @@ required-features = ["boolean", "shortint", "internal-keycache"] | |||
|
|||
# Real use-case examples | |||
|
|||
[[example]] | |||
name = "demo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zk-pok-demo maybe ?
@@ -241,6 +241,7 @@ impl ServerKey { | |||
let counter_num_blocks = ((num_bits_in_ciphertext - 1).ilog2() + 1 + 1) | |||
.div_ceil(self.message_modulus().0.ilog2()) as usize; | |||
|
|||
// 11111000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was explaining something via zoom and used that file as a whiteboard
e337007
to
88cbc2f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the core part there are some confusions between parallel and non parallel variants it seems, and some debug code is still there, I marked the biggest ones.
Also the private param of the CRS should not be available in the CRS structure otherwise I believe the security of the ZK falls, ping me if you need help
fn next_u32(&mut self) -> u32 { | ||
let mut bytes = [0u8; std::mem::size_of::<u32>()]; | ||
bytes.iter_mut().for_each(|b| *b = self.generate_next()); | ||
u32::from_ne_bytes(bytes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the uniform generation from the already implemented Uniform distribution if needed or use from_le_bytes, I believe from_le_bytes was preferred to avoid endianness issues cross platform
tfhe/src/core_crypto/commons/math/random/uniform.rs
fn next_u64(&mut self) -> u64 { | ||
let mut bytes = [0u8; std::mem::size_of::<u64>()]; | ||
bytes.iter_mut().for_each(|b| *b = self.generate_next()); | ||
u64::from_ne_bytes(bytes) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same thing as above
} | ||
} | ||
|
||
impl<T> BoundedDistribution<T, T::Signed> for TUniform<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't low supposed to be equal to high given the trait definition above ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, Low = High
in the trait definition is just to set Low as by default being the same type as High, we it can be anything else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok got it, we may want to be consistent here and have everything with a single type wdyt ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can
fn low_bound(&self) -> Bound<T::Signed> { | ||
Bound::Included(self.min_value_inclusive()) | ||
} | ||
|
||
fn high_bound(&self) -> Bound<T> { | ||
Bound::Included(self.max_value_inclusive().into_unsigned()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason here to have different signedness for both bounds ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for flexibility
<input | ||
type="button" | ||
id="compactPublicKeyZeroKnowledgeBench" | ||
value="Compact zk bench" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think nit needs bench with a capital B to be picked up by our filter and I had issues with that last week
you adjusted time outs right ?
slice_wrapping_add_assign(output_mask.as_mut(), mask_noise); | ||
|
||
// Fill the body chunk afterward manually as it most likely will be smaller than | ||
// the full convolution result. b convolved r + Delta * m + e2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
careful with the comment and the reverse we did, as we can see above I added a rev at some point in the comment to match the zk order, the comment seems to be missing this ?
// Fill the temp buffer with b convolved with r | ||
// // Fill the temp buffer with b convolved with r |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary diff
let zk_body_convolved = polymul_rev(pk_body.as_ref(), binary_random_slice); | ||
|
||
assert_eq!(&pk_body_convolved, &zk_body_convolved); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debug can be removed
// Fill the body chunk afterwards manually as it most likely will be smaller than | ||
// the full convolution result. rev(b convolved r) + Delta * m + e2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should keep this comment here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is still here I believe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing the rev(...)
); | ||
} | ||
|
||
/// Encrypt and generates a zero-knowledge proof of an input cleartext list in an output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parallel variant of
c6cfc6e
to
efd9efb
Compare
efd9efb
to
6177cf4
Compare
>( | ||
lwe_compact_public_key: &LweCompactPublicKey<KeyCont>, | ||
output: &mut LweCompactCiphertextList<OutputCont>, | ||
messages: &InputCont, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missed that during review
https://github.com/zama-ai/tfhe-rs-internal/issues/520
don't want to break things now, too late to make small fixes, but I think we should stick to plaintexts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or whatever type + encoding information
f4727ea
to
6175668
Compare
6175668
to
a67f6a6
Compare
- integration of work done by Sarah in the repo Co-authored-by: sarah el kazdadi <[email protected]>
a67f6a6
to
7151550
Compare